Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update plugin-getter version matching check #12953

Conversation

nywilken
Copy link
Contributor

@nywilken nywilken commented May 3, 2024

This change is an attempt to remove the need for additional temporary files, along with calls to stat the temp files, to reduce the number of files being created, opened, and closed. Instead of creating a temp file for the binary we download it into the expected directory if after validation we find that the file is invalid or has a mis-matched version the file is deleted.

In addition to this change, the logic for falling back to a previous version if the highest matched version is a pre-release has been removed. Instead we will assume that any prior versions will exhibit the same issue and return immediately. A user can install the version manually if they will or they can modify their version constraint to a properly released version.

@nywilken nywilken requested a review from a team as a code owner May 3, 2024 18:25
@nywilken
Copy link
Contributor Author

nywilken commented May 3, 2024

I find that writing directly into the plugin directory and removing if the binary invalid is a bit finicky when it comes to the test. I’m going to update this to include your temporary file writing changes and then have it do a rename instead of opening and copying a new file.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple comments, but I think the approach is valid, not sure it'll reduce the amount of syscalls depending on the implementation of Rename, but it very well might.
Let's iterate on it a bit before merging into the harden source branch

packer/plugin-getter/plugins.go Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
packer/plugin-getter/plugins.go Show resolved Hide resolved
packer/plugin-getter/plugins.go Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
packer/plugin-getter/plugins.go Outdated Show resolved Hide resolved
@nywilken nywilken force-pushed the nywilken/harden_remote_plugins_consolidation branch 2 times, most recently from 50ed7cd to 5effa77 Compare May 7, 2024 02:36
@nywilken nywilken force-pushed the harden_remote_plugin_installs branch from 1a37f84 to 709fdcd Compare May 7, 2024 02:37
@nywilken nywilken force-pushed the nywilken/harden_remote_plugins_consolidation branch 2 times, most recently from b6e8a4d to 269920b Compare May 7, 2024 10:25
@@ -976,6 +974,12 @@ func CheckVersion(filename string, identifier string, version *goversion.Version
err := fmt.Errorf("binary reported version (%q) is different from the expected %q, skipping", desc.Version, version.String())
return &ContinuableInstallError{Err: err}
}
if version.Prerelease() != "" {
Copy link
Contributor Author

@nywilken nywilken May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases where a plugin is tagged and released as a prerelease but the describe version is actually a final version (e.g github.com/hashicorp/hashicup@v1.2.0-dev) Packer should prevent the installation because of a mismatch version that will not load properly.

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plugin "github.com/hashicorp/hashicups" release v1.0.2-dev binary reports version "1.0.2-dev". This is likely an upstream issue.

Yeah I get this on my latest harden branch, so my original hunch that we got the candidates through a Constraint is not right it seems. IMHO, we should reject remotely installing non-final versions of a plugin, even before we even try to contact Github, so we should check the version requested, and only accept final versions for commands like plugins install or packer init.

@nywilken nywilken force-pushed the nywilken/harden_remote_plugins_consolidation branch from ade8d2f to f67da39 Compare May 8, 2024 00:40
nywilken added 2 commits May 8, 2024 15:12
This change is an attempt to remove the need for additional temporary files, along with
calls to stat the temp files, to reduce the number of files being created, opened, and closed.
In addition to this change, the logic for falling back to a previous version if the highest matched version
is a pre-release has been removed. Instead we will assume that any prior versions will exhibit the same issue and
return immediately. A user can install the version manually if they will or they can modify their version constraint
to a properly released version.
```
~>  packer init mondoo_req.pkr.hcl
Failed getting the "github.com/mondoohq/cnspec" plugin:
error:
Remote installation of the plugin version 10.8.1-dev is unsupported.
This is likely an upstream issue with the 10.8.1 release, which should be reported.
If you require this specific version of the plugin, download the binary and install it manually.

packer plugins install --path '<plugin_binary>' github.com/mondoohq/cnspec

```
@nywilken nywilken force-pushed the nywilken/harden_remote_plugins_consolidation branch from f67da39 to 34cb984 Compare May 8, 2024 19:13
nywilken added 3 commits May 8, 2024 15:13
* Only create plugin directories if there is  potential plugin install
… a prerelease

* Refactor InstallError string messages
@nywilken nywilken force-pushed the nywilken/harden_remote_plugins_consolidation branch from 34cb984 to 66ef512 Compare May 8, 2024 19:14
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lbajolet-hashicorp lbajolet-hashicorp merged commit e5434bf into harden_remote_plugin_installs May 8, 2024
11 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the nywilken/harden_remote_plugins_consolidation branch May 8, 2024 19:48
Copy link

github-actions bot commented Jun 8, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants